Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

source-mysql: fix regression involving boolean values in CDC #51519

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

source-mysql: fix regression involving boolean values in CDC #51519

wants to merge 1 commit into from
+245 −255

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Jan 13, 2025 •
edited
Loading

What

Prompted by https://github.com/airbytehq/oncall/issues/7242

A customer got the following message when upgrading to 3.10.0, which included a lot of converter changes.

source ERROR blc-localhost:41327 i.d.c.b.BinlogStreamingChangeEventSource(logStreamingSourceState):1145 Error during binlog processing. Last offset stored = ...
source ERROR blc-localhost:41327 i.d.p.ErrorHandler(setProducerThrowable):52 Producer failure io.debezium.DebeziumException: Error processing binlog event
....
Caused by: org.apache.kafka.connect.errors.DataException: Invalid value: null used for required field: "active", schema type: BOOLEAN

This PR aims to fix this by replacing our own boolean handling logic with Debezium's. Unfortunately, I was not able to reproduce this error.

How

Replaces MySqlSourceCdcBooleanConverter with TinyIntOneToBooleanConverter. Adds missing test cases.

Review guide

none

User Impact

Hopefully this fixes the issue. This PR is required to find out.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@vercel Vercel
Copy link

vercel bot commented Jan 13, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 9:27pm

@postamar postamar force-pushed the postamar/source-mysql-fix-boolean-issue branch from 0511eef to 097e577 Compare January 13, 2025 15:04
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jan 13, 2025
@theyueli theyueli requested a review from rodireich January 15, 2025 19:24
@theyueli
Copy link
Contributor

@rodireich will follow up on this issue as oncall.

rodireich
rodireich reviewed Jan 21, 2025
View reviewed changes
...-mysql/src/main/kotlin/io/airbyte/integrations/source/mysql/MySqlSourceDebeziumOperations.kt
MySqlSourceCdcTemporalConverter::class
)
.with("converters", "tinyint-one,temporal")
.with("tinyint-one.type", TinyIntOneToBooleanConverter::class.qualifiedName!!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to set a custom converter when it's debezium's own?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still have to opt in to it, it's not Debezium's default behaviour.

@postamar postamar force-pushed the postamar/source-mysql-fix-boolean-issue branch from 097e577 to 103d967 Compare January 30, 2025 21:21
@postamar
Copy link
Contributor Author

@airbytehq/dbsources FYI this remains untested as of today, for lack of a suitable test candidate. See oncall ticket for details.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Marius Posta seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodireich rodireich rodireich left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/mysql
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@postamar @theyueli @CLAassistant @rodireich @octavia-squidington-iii